Expose onchain transactions in store #432

tnull · 2025-01-16T13:28:13Z

Closes #67

Previously, onchain-transactions where not tracked in our PaymentStore.
Now, that the upgrade to BDK 1.0 is behind us, we take a stab at finally exposing them in our interface via PaymentStore.

In the future we're looking to add dedicated PaymentKinds for channel fundings/channel closes (see #447), but for now all are exposed under the Onchain umbrella kind.

tnull · 2025-01-27T15:57:10Z

As we currently don't have a good wait to discern closing transactions, I now split out the event emission to #448 which will land at a later date. Going ahead and undrafting this.

src/payment/store.rs

jkczyz · 2025-01-28T02:53:02Z

src/wallet/mod.rs

+			// TODO: It would be great to introduce additional variants for
+			// `ChannelFunding` and `ChannelClosing`. For the former, we could just
+			// take a reference to `ChannelManager` here and check against
+			// `list_channels`. But for the latter the best approach is much less
+			// clear: for force-closes/HTLC spends we should be good querying
+			// `OutputSweeper::tracked_spendable_outputs`, but regular channel closes
+			// (i.e., `SpendableOutputDescriptor::StaticOutput` variants) are directly
+			// spent to a wallet address. The only solution I can come up with is to
+			// create and persist a list of 'static pending outputs' that we could use
+			// here to determine the `PaymentKind`, but that's not really satisfactory, so
+			// we're punting on it until we can come up with a better solution.


I wonder if this gets more complicated with splicing in/out or more exotic transactions paying us and opening a channel, for instance.

Hmm, good point, now opened the more lightningdevkit/rust-lightning#3566 to supersede lightningdevkit/rust-lightning#3548

Would be great if an API allowing to query the type of a given transaction (Txid) could be added as part of the splicing work.

jkczyz · 2025-01-28T02:53:58Z

src/wallet/mod.rs

+			let (direction, amount_msat) = if sent > received {
+				let direction = PaymentDirection::Outbound;
+				let amount_msat = Some(sent.to_sat().saturating_sub(received.to_sat()) * 1000);
+				(direction, amount_msat)
+			} else {
+				let direction = PaymentDirection::Inbound;
+				let amount_msat = Some(received.to_sat().saturating_sub(sent.to_sat()) * 1000);
+				(direction, amount_msat)
+			};


Should we consider these two separate payments? I guess we could't use the txid as the payment id, though.

Mhh, given that (IIUC) received includes the change, I'd rather not add each change output as an inbound payment?

jkczyz · 2025-01-28T03:02:40Z

src/wallet/mod.rs

+					let payment_status = if cur_height >= confirmation_height + ANTI_REORG_DELAY - 1
+					{
+						PaymentStatus::Succeeded
+					} else {
+						PaymentStatus::Pending
+					};


Say the transaction was re-orged out and was then RBF'ed. Is that possible? Would we have payment stuck in pending?

Hmm, that's a very valid concern. Tbh. I'm not entirely sure how to handle that: for one, the RBF'd transaction is technically pending as there could be another reorg that makes it part of the best chain again. So essentially any signed & broadcasted transaction remains 'pending' forever.

I think the only way I can come up with to handle this would be to never add Pending transactions but jump to Succeeded directly once they reach ANTI_REORG_DELAY. Or we just keep the current approach and add docs noting that transactions in pending might never succeed for one reason or another?

I think at least for BDK's own RBF's we could see if we find a better solution when we do #367.

Could we possibly detect that the UTXO has been spent and remove the pending payment?

I guess, but that would require us to keep track and persist a list of all the UTXOs, write an adapter around lightning-transaction-sync's Filter implementation to also register spends for them, and then implement Confirm on Wallet, just to be able to remove any Pending entries that have been replaced.

For now that seems like a lot of overhead to just account for this rare edgecase (reorg + RBF)? But I wouldn't completely rule it out to do something like it in if we find we need to do additional RBF tracking anyways as part of #367.

jkczyz · 2025-01-28T03:04:42Z

src/wallet/mod.rs

+			// here to determine the `PaymentKind`, but that's not really satisfactory, so
+			// we're punting on it until we can come up with a better solution.
+			let kind = crate::payment::PaymentKind::Onchain { txid, status: confirmation_status };
+			let (sent, received) = locked_wallet.sent_and_received(&wtx.tx_node.tx);


Is change abstracted away from this API?

I don't think so, the docs state:

/// This method returns a tuple `(sent, received)`. Sent is the sum of the txin amounts /// that spend from previous txouts tracked by this wallet. Received is the summation /// of this tx's outputs that send to script pubkeys tracked by this wallet.

so IIUC received would include the change.

Couldn't the change be more than the amount sent, making it look like an inbound payment when it was really an outbound? Or am I misunderstanding?

Couldn't the change be more than the amount sent, making it look like an inbound payment when it was really an outbound? Or am I misunderstanding?

IIUC, any change amount would be accounted for both in sent and received, so you can essentially ignore it in terms of comparisons (think: subtract on both sides).

Say you send a payment of 10 sats, and add a txin spending an wallet output with 100 sats. This would result in two txouts, one to the recipient with 10 sats, one change back to a wallet address with 85sats or so, leaving 5 for mining fees. IIUC, this would result in sent = 100, received = 85, resulting in sent > received => PaymentDirection::Outbound.

jkczyz

Feel free to squash

jkczyz · 2025-01-29T22:46:41Z

src/wallet/mod.rs

+					let payment_status = if cur_height >= confirmation_height + ANTI_REORG_DELAY - 1
+					{
+						PaymentStatus::Succeeded
+					} else {
+						PaymentStatus::Pending
+					};


Could we possibly detect that the UTXO has been spent and remove the pending payment?

Here, we move updating fields via `PaymentDetailsUpdate` to `PaymentDetails::update`. This allows us to only update the fields that changed, keeping track *whether* something changed, and only updating the timestamp and persisting the entry *if* something changed. This is a nice improvement in general (as we want to reduce persist calls anyways), but we'll also use this for batch updates in the next commits.

We implement an inserting/updating method that will only persist entries that have been changed.

tnull · 2025-01-30T12:57:37Z

Rebased to resolve minor conflicts

Previously, `PaymentKind::Onchain` was simply a placeholder entry we never actually used. Here, we extend it to include fields that are actually useful.

We update the payment store whenever syncing the wallet state finished.

tnull · 2025-01-30T16:03:54Z

Squashed the fixup without further changes.

danielnordh · 2025-01-30T17:10:41Z

Yes, thank you for getting this done!!

tnull marked this pull request as draft January 16, 2025 13:28

tnull added this to the 0.5 milestone Jan 16, 2025

tnull mentioned this pull request Jan 23, 2025

LDK Node v0.5 Tracking Issue #360

Open

10 tasks

tnull force-pushed the 2025-01-include-onchain-transactions-in-store branch 2 times, most recently from b959830 to 306a78e Compare January 27, 2025 15:45

tnull changed the title ~~Expose onchain transactions in store and events~~ Expose onchain transactions in store Jan 27, 2025

This was referenced Jan 27, 2025

Expose on-chain transactions in events #446

Open

Create new PaymentKind variants for channel funding/closing on-chain transactions #447

Open

Include onchain transactions in events #448

Draft

tnull marked this pull request as ready for review January 27, 2025 15:57

tnull force-pushed the 2025-01-include-onchain-transactions-in-store branch 2 times, most recently from 585e14f to 6437515 Compare January 27, 2025 16:45

tnull requested a review from jkczyz January 27, 2025 16:46

tnull force-pushed the 2025-01-include-onchain-transactions-in-store branch from 6437515 to 403be03 Compare January 27, 2025 17:03

jkczyz reviewed Jan 28, 2025

View reviewed changes

tnull force-pushed the 2025-01-include-onchain-transactions-in-store branch 2 times, most recently from 1d05f9c to 993bf0b Compare January 28, 2025 10:50

jkczyz reviewed Jan 29, 2025

View reviewed changes

tnull added 3 commits January 30, 2025 13:55

Add more debug_asserts for updating BOLT12 payment hashes

d92a568

Prefactor: Implement PaymentStore::insert_or_update

c9ddfae

We implement an inserting/updating method that will only persist entries that have been changed.

tnull force-pushed the 2025-01-include-onchain-transactions-in-store branch from 993bf0b to da45da3 Compare January 30, 2025 12:56

jkczyz approved these changes Jan 30, 2025

View reviewed changes

tnull added 3 commits January 30, 2025 17:03

Add required fields to PaymentKind::Onchain

62ff60c

Previously, `PaymentKind::Onchain` was simply a placeholder entry we never actually used. Here, we extend it to include fields that are actually useful.

Have Wallet take PaymentStore ref

e46e407

Update PaymentStore after each Wallet sync

0710434

We update the payment store whenever syncing the wallet state finished.

tnull force-pushed the 2025-01-include-onchain-transactions-in-store branch from da45da3 to 0710434 Compare January 30, 2025 16:03

tnull merged commit aaf74cc into lightningdevkit:main Jan 30, 2025
5 of 14 checks passed

tnull mentioned this pull request Jan 30, 2025

Find way to drop RBF'd Pending payments #452

Open

reez mentioned this pull request Feb 3, 2025

(draft) feat: onchain reez/Monday#161

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose onchain transactions in store #432

Expose onchain transactions in store #432

tnull commented Jan 16, 2025 •

edited

Loading

tnull commented Jan 27, 2025

jkczyz Jan 28, 2025

tnull Jan 28, 2025

jkczyz Jan 28, 2025

tnull Jan 28, 2025

jkczyz Jan 28, 2025

tnull Jan 28, 2025

jkczyz Jan 29, 2025

tnull Jan 30, 2025 •

edited

Loading

jkczyz Jan 28, 2025

tnull Jan 28, 2025

jkczyz Jan 28, 2025

tnull Jan 28, 2025

jkczyz left a comment

jkczyz Jan 29, 2025

tnull commented Jan 30, 2025

tnull commented Jan 30, 2025

danielnordh commented Jan 30, 2025

Expose onchain transactions in store #432

Expose onchain transactions in store #432

Conversation

tnull commented Jan 16, 2025 • edited Loading

tnull commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Jan 30, 2025

tnull commented Jan 30, 2025

danielnordh commented Jan 30, 2025

tnull commented Jan 16, 2025 •

edited

Loading

tnull Jan 30, 2025 •

edited

Loading